Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed toPersistentSet, toPersistentMap to work with any impl. #108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed toPersistentSet, toPersistentMap to work with any impl. #108

wants to merge 1 commit into from

Conversation

comahe-de
Copy link

toPersistentSet and toPersistentMap now working in a similar way as toPersistentList

The contract in the KDoc was not fulfilled as it says for toPersistentSet:

If the receiver is already a persistent set,
returns it as is.
If the receiver is a persistent set builder,
calls build on it and returns the result.

But the previous implementation just checked for
PersistentOrderedSet, other implementations
of PersistentSet where not considered

Similar problem war fixed for toPersistentMap

@MarcelHeckel
Fixed toPersistentSet, toPersistentMap to work with any impl.
8c41c3c 
`toPersistentSet` and `toPersistentMap` now working in a similar way as `toPersistentList`

The contract in the KDoc was not fulfilled as it says for `toPersistentSet`:

> If the receiver is already a persistent set,
> returns it as is.
> If the receiver is a persistent set builder,
> calls `build` on it and returns the result.

But the previous implementation just checked for
`PersistentOrderedSet`, other implementations
of `PersistentSet` where not considered

Similar problem war fixed for `toPersistentMap`
@qurbonzoda qurbonzoda self-requested a review May 14, 2021 01:48
@qurbonzoda
Copy link
Contributor

toPersistentSet/Map intensionally checks only for the ordered set.
The idea here is to be consistent with stdlib toMutableSet(), which always returns an ordered set.

Can you please share your use case and why working with any implementation is a better behavior?

@comahe-de
Copy link
Author

@qurbonzoda I have some custom implementations of PersistentList/-Set/-Map that mainly delegate to the default PersistentList/-Set/-Map but add some additional properties. While a call to toPersistentList() on my PersistentList implementation returns the same object, a call to toPersistentSet() on my PersistentSet implementation returns a new object, which was really unexpected as the doc says If the receiver is already a persistent set, returns it as is.

Regarding toMutableSet(): this method has a complete other behaviour: it always returns a new Set even if is already a Set. So the name is not well chosen. toNewMutableSet() would be a better name. But this is a another topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qurbonzoda qurbonzoda Awaiting requested review from qurbonzoda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@comahe-de @qurbonzoda @MarcelHeckel